Import inside function body

import re is placed inside detect_statement_type rather than at the top of the module. This violates the project's import placement rule.

Actually, since from __future__ import annotations must be the first statement, the import re should be placed after the existing imports at the top of the module (after line 13).

Rule Used: Python imports should always be placed at the top ... (source)

Learnt From
ethyca/fides#6536

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

Done — moved import re to module-level imports.

Import inside function body

from fides.api.util.cache import get_cache is placed inside build_identity_resolver rather than at the top of the module. This import should be moved to the top-level imports section.

Then update the function signature to remove the local import.

Rule Used: Move imports to the top of the module rather than ... (source)

Learnt From
ethyca/fidesplus#2441

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

Done — moved from fides.api.util.cache import get_cache to module-level imports.

Hardcoded magic string for control type

The string "purpose_restriction" is a magic string used as a control type identifier without a named constant. Per the project convention, define this as a named constant with an explanatory comment.

Then reference it via control=CONTROL_PURPOSE_RESTRICTION at the call site.

Rule Used: Define magic strings as named variables with expla... (source)

Learnt From
ethyca/fidesplus#2515

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

Done — extracted "purpose_restriction" to a PBAC_CONTROL_TYPE module-level constant.

Single-character and abbreviated variable names

Variables v (line 265) and dk (lines 127, 130, 131, 190, 191) are 1–2 character names that should use full descriptive names per the project's naming convention. Consider renaming dk to dataset_key and v to violation throughout these methods.

Rule Used: Use full names for variables, not 1 to 2 character... (source)

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

Done — renamed dk to dataset_key and v/pk to violation/purpose_key throughout.

Incomplete dataset purpose lookup causes silent non-enforcement

This method populates every dataset with an empty purpose_keys frozenset. Per Rule 3 in the evaluation engine ("if a dataset has no declared purposes, access is compliant"), this means all consumers with at least one declared purpose will always receive a compliant result, regardless of the actual dataset's allowed purposes.

Concretely: a consumer with purpose "marketing" accessing a dataset annotated for "billing" will not produce a violation, because the lookup returns empty rather than frozenset({"billing"}).

Violations are only generated today for consumers with zero declared purposes (Rule 1). Registered consumers with purposes bypass all checks unless dataset_purpose_overrides is explicitly provided on every call.

The intended implementation should fetch declared purposes from the Fides dataset catalog (fideslang annotations on datasets, collections, and fields). Please add a clear docstring or inline comment documenting this limitation so callers are not misled into thinking enforcement is active.

Done — _build_dataset_purposes now uses a pre-built dataset_purposes map passed at init time, so purpose-overlap enforcement works in the OSS path. Added both unit and integration tests.

Critical: sqlglot is not declared as a dependency.

sqlglot is imported at the top of this file but does not appear in pyproject.toml. Any environment that installs fides without it will get an ImportError at module load time — not a lazy failure.

Add sqlglot to the appropriate dependency group in pyproject.toml before merging.

Done — added sqlglot~=30.0.3 to pyproject.toml dependencies and removed the type: ignore[import-not-found] comments from the imports.

Suggestion: silent failure on parse errors loses observability.

A bare except Exception: return refs means malformed SQL silently produces an empty table-reference list. Downstream, this will look like a compliant query (no datasets accessed) rather than an evaluation error, making bugs very hard to diagnose.

At minimum, add a logger.debug or logger.warning here so operators can see when queries fail to parse.

Done — added logger.warning with exc_info=True so parse failures are visible in logs instead of silently returning an empty list.

Suggestion: import re at function scope instead of module level.

re is a stdlib module and should be imported at the top of the file alongside other imports. Importing inside a function is unconventional and mildly confusing — it implies a lazy-import rationale that doesn't apply here.

Done — moved import re to module-level imports.

Critical: _build_dataset_purposes always returns empty purpose sets — PBAC violations will never be produced for consumers that have purposes.

By Rule 3 in evaluate.py: "If a dataset has NO declared purposes, access is considered compliant." This method always returns purpose_keys=frozenset() for every dataset, so unless dataset_purpose_overrides is passed by the caller, every consumer-with-purposes will always get a clean result regardless of what the dataset is.

The only case that still produces violations is Rule 1 (consumer has no purposes at all). The core PBAC purpose-overlap check (Rule 2) is effectively dead code in the default evaluation path.

If fidesplus is expected to supply purpose data via dataset_purpose_overrides, that assumption should be documented prominently here — and ideally the method should be overridable or accept a dataset-purpose lookup callable so the OSS service itself is useful.

Done — _build_dataset_purposes now uses a pre-built dataset_purposes map passed at init time. The caller is responsible for building the map from the dataset catalog (including collection-level and field/sub-field-level purposes). Keys not in the map fall back to empty purposes. Added unit tests for evaluate_access() covering all three rules plus collection inheritance, and integration tests for InProcessPBACEvaluationService.evaluate() covering registered/unregistered consumers, the init-time map, overrides precedence, and backward compat.

Suggestion: read-then-write race condition in index maintenance.

The pattern here reads the existing entity with self._cache.get(key) (line 74) outside the pipeline, then queues the stale-index cleanup and new-index writes inside the pipeline. Between the get and pipe.execute(), another writer can mutate or delete the entity, leaving indexes in an inconsistent state.

Redis doesn't support optimistic locking via WATCH/MULTI/EXEC natively through a pipeline, but using WATCH + a MULTI/EXEC block would make this safe. Alternatively, document the known race as acceptable given the expected write throughput for this use case.

Acknowledged — this is a known limitation at current write concurrency levels. The race window is narrow (single Redis round-trip) and the consequence is stale secondary indexes, not data loss. Will document and revisit if concurrent writes become a real pattern.

Suggestion: list_page loads all PKs into memory to paginate.

_resolve_pks calls smembers on the :_all key (or performs a sinter), loading every PK in the set into a Python list before slicing. For large consumer/purpose sets this will be slow and memory-intensive. Redis SSCAN with cursor-based iteration would avoid materializing the full set, though it makes consistent ordering harder. Worth noting as a known limitation at this stage.

Acknowledged as a known limitation. At current expected collection sizes this is acceptable. Will revisit with cursor-based pagination if collections grow large enough to cause memory pressure.

Suggestion: _validate_purpose_keys raises ValueError instead of a domain exception.

Every other error path in this repository raises ResourceNotFoundError or ResourceAlreadyExistsError from fides.service.pbac.exceptions. A ValueError here breaks that pattern — callers (and route handlers) that catch domain exceptions will miss this. Consider raising ResourceNotFoundError("DataPurpose", fk) for consistency.

Done — replaced ValueError with ResourceNotFoundError("DataPurpose", fk) for consistency with other error paths.

Suggestion: members-based resolution is documented but not wired up.

The README and BasicIdentityResolver both document a three-step resolution chain: contact_email → external_id → members list. RedisIdentityResolver only implements the first two. The members index is not maintained in DataConsumerRedisRepository._get_index_entries, so there is no way to find a consumer by member email via Redis.

Either remove the members resolution step from the README's "Resolution chain" section for the OSS path, or add a members index to the consumer repository and implement the lookup here.

Done — updated README to note that members-based resolution is not yet implemented in the OSS path. The resolution chain section now reflects what RedisIdentityResolver actually supports (contact_email and external_id only).

Nice to have: Rule 1 asymmetry with Rule 3 — consider documenting intent.

Rule 1 fires a violation when a consumer has no purposes, even when the dataset also has no declared purposes (effective == frozenset()). Rule 3, by contrast, says that a consumer with purposes accessing a dataset with no purposes is compliant.

This asymmetry means "consumer with no purposes + dataset with no purposes" = violation, while "consumer with purposes + dataset with no purposes" = compliant. That may be intentional (a consumer must declare something to access any dataset), but it's subtle enough to warrant a comment in the code or the docstring explaining why.

Done — added a comment explaining the intentional asymmetry: an unregistered consumer (no purposes) should never silently pass evaluation, even against an unrestricted dataset.

Suggestion: the filtering logic under test is duplicated from production code.

_filter_violations replicates the body of InProcessPBACEvaluationService._filter_violations_through_policies verbatim. If the production implementation changes, these tests will silently continue to pass against the copy rather than the real code.

Consider testing the method on the actual service instance (with a test double for the cache), or at minimum extract the filtering function to a standalone helper that both the service and the tests import.

Acknowledged — deferring this refactor to a follow-up. The duplicated logic is a test isolation choice; will revisit when the service interface stabilizes.